It's better to call SanitizeCredentialURLs in EventFormatTextMessage, then all messages are sanitized, nothing will be missing. And it's easier to add a test for EventFormatTextMessage

It's better to call SanitizeCredentialURLs in EventFormatTextMessage, then all messages are sanitized, nothing will be missing. And it's easier to add a test for EventFormatTextMessage

That seems more secure, but again this would be adding a few bit of lines to all the log statements and for the majority of time we may not even need it. Is this extra cost acceptable ?

It's better to call SanitizeCredentialURLs in EventFormatTextMessage, then all messages are sanitized, nothing will be missing. And it's easier to add a test for EventFormatTextMessage

That seems more secure, but again this would be adding a few bit of lines to all the log statements and for the majority of time we may not even need it. Is this extra cost acceptable ?

SanitizeCredentialURLs is designed to be very fast. If the message doesn't look like containing a URL with password, (containing :// and @), it does nothing.

Personally I think it's worth to make the logger system more secure, but I am fine with either (current approach, or calling SanitizeCredentialURLs for all log messages). It's up to the reviewers. @lunny @techknowlogick

I think as a quick patch, this is enough.

Adding a log level check requires balancing performance and security. Most log format arguments don’t need such checks, but verifying the log level can help prevent potential leaks of sensitive information.

Alternatively, we could introduce a dedicated type, for example, type SensitiveString string, to explicitly mark and handle sensitive data. This approach would provide a reasonable balance between performance and security. These improvements could be introduced in a separate PR.

I sent #35594 as an example how it might be.